fix(download): resolve SaveGLB 3d/mesh outputs in extract_output_entries (BE-4417) - #600
Conversation
…ies (BE-4417)
comfy download <prompt_id> --where cloud returned download_no_outputs for
succeeded partner-3D jobs (Tripo 3.1, Rodin Gen 2.5): the SaveGLB node emits
its entries under the "3d" key (ui={"3d": results}), but extract_output_entries
only scanned the fixed allowlist (images, gifs, videos, audio, files), so the
.glb was never resolved and transfer.py emitted download_no_outputs.
Replace the hardcoded allowlist with shape-based detection mirroring ComfyUI
core's normalize_outputs and the cloud worker's isFileOutputArray: any
node-output key whose value is a list of dicts carrying a filename is treated
as file outputs, with "animated" explicitly skipped (it emits (True,) flags).
This is a strict superset of the prior five keys and fixes both the
job-watcher state-file path and the API fallback, which share the extractor.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 2 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…lder,type) tuple (BE-4417) A node that surfaces the same artifact under two keys — e.g. the cloud worker's singular "video" alongside the classic plural "videos" — would emit two identical /view URLs and download the same file twice. De-dup on the full entry tuple, first occurrence wins, preserving order. Addresses the Cursor review de-dup finding on #600. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bigcat88
left a comment
There was a problem hiding this comment.
Approving. Premise verified in ComfyUI source, and the "strict superset" claim proven differentially rather than argued.
The premise is real. comfy_extras/nodes_save_3d.py:406 — return IO.NodeOutput(ui={"3d": results}). SaveGLB genuinely files under "3d", which the old five-key allowlist could never see.
Shape-based detection mirrors core, and I checked core rather than taking the docstring's word. comfy_execution/jobs.py:82-107 — normalize_outputs iterates every media-type key generically and short-circuits on if media_type == 'animated' or not isinstance(items, list). That's the same two guards, in the same order. The explicit "animated" skip isn't redundant belt-and-braces either: I fed it a hypothetical dict-shaped animated: [{"filename": ...}] and it's correctly skipped, which the filename guard alone would not have done.
Strict superset, proven by differential. Same inputs through main's extractor and this one:
| case | main | this PR |
|---|---|---|
images / gifs / videos / audio / files |
resolved | identical |
| multi-node | resolved | identical |
3d |
[] |
m.glb resolved |
Not one classic bucket moved, including subfolder and non-output type values. That's the property that matters for a change like this.
Full shape matrix behaves:
SaveGLB 3d -> abc123.glb singular video -> v.mp4
future key 'mesh' -> m.obj all 5 classic -> 5 entries
animated [True] -> skipped (sibling image still found)
text / dims / non-list / None items -> correctly filtered
video + videos same file -> deduped to 1
That last row is a genuinely good catch. The cloud worker's singular "video" alongside the classic plural would otherwise have produced two identical /view URLs and downloaded the same artifact twice — a bug the old allowlist accidentally hid, and one that shape-based detection would have introduced without the (node_id, filename, subfolder, type) dedup.
Tests are non-vacuous. Reverting the guard to the hardcoded allowlist fails exactly the three that should:
test_extract_resolves_saveglb_3d_key, test_extract_resolves_singular_video_key, and the end-to-end TestSaveGlb3dDownload::test_3d_only_record_downloads_glb.
Full suite: 3305 passed, 7 skipped, 1 failed — test_non_fast_deps_uses_global_python, the sandbox artifact that fails on plain main here too. ruff check clean.
Forward-compatibility is the real win: an unrecognised future key like mesh resolves automatically, so the next output type won't need a code change. And since both the job-watcher state-file path and the transfer.py API fallback share this extractor, one fix covers both.
On the scope call — leaving the two jobs.py allowlist copies (jobs list output count ~L325, jobs watch executed handler ~L1195) alone is right for this diff, and I appreciate it being named rather than silently left. They will show the same 3D-blindness for local jobs, so it's worth a follow-up ticket: a user who renders a mesh locally still sees a wrong output count in jobs list. Converging all three on this extractor is the obvious end state.
ELI-5
When you run a workflow that makes a 3D model (a SaveGLB node, e.g. Tripo 3.1 or Rodin Gen 2.5), ComfyUI files the finished
.glbunder a bucket labelled"3d". Butcomfy download <prompt_id> --where cloudonly knew to look in the buckets labelledimages,gifs,videos,audio, andfiles— so it never found the mesh and gave up withdownload_no_outputs, even though the file was sitting right there in Media Assets. This teaches the downloader to recognise a file by its shape (it's a list of records that have afilename) instead of by a fixed list of bucket names, so 3D/mesh outputs — and any future output key — resolve and download.What changed
extract_output_entriesincomfy_cli/comfy_client.pypreviously scanned a hardcoded allowlist of node-output keys:("images", "gifs", "videos", "audio", "files"). SaveGLB emits its entries under"3d"(ui={"3d": results}in ComfyUI core'snodes_save_3d.py), so those entries were dropped,extract_output_urlsreturned[], andtransfer.pyreporteddownload_no_outputs.The allowlist loop is replaced with shape-based detection, mirroring ComfyUI core's
normalize_outputsand the cloud worker'sisFileOutputArray: iterate every node-output key, and for any key whose value is a list, collect the items that are dicts carrying a"filename". The"animated"key is skipped explicitly (core emits it as(True,)boolean flags, not file entries — the dict guard would already drop it, but the explicit skip matches core semantics and guards against future dict-shaped animated metadata). Keys holding non-file lists ("text"strings,"dims"dicts without a filename, etc.) are naturally filtered by theisinstance(item, dict) and "filename" in itemguard.This is a strict superset of the prior behaviour for the five existing keys — the
(filename, subfolder, type)join-triple contract is unchanged. The only observable difference is intra-node ordering: entries within a single node now follow node-output key insertion order rather than the old fixed key order (the docstring's ordering contract is updated accordingly). No test depends on cross-key ordering within a single node.Because both the job-watcher state-file path (
job_watcher.py) and the API fallback (transfer.py) share this one extractor, this fixes both.Tests
test_extract_resolves_saveglb_3d_key— a"3d"-keyed record resolves to the entry and builds/view?filename=abc123.glb&subfolder=3d&type=output.test_extract_resolves_singular_video_key— the cloud worker's singular"video"key (fromsynthesizeMediaTypeFromResult) resolves.test_extract_skips_non_file_shaped_keys—"animated": [True],"text": ["hello"],"dims": [{"width": 512}]produce no entries.TestSaveGlb3dDownload.test_3d_only_record_downloads_glb— regression throughcomfy download: a 3d-only history record downloads a file with a.glbextension instead of erroring.Full suite:
2773 passed, 37 skipped.ruff check+ruff format --checkclean.Judgment call (out of scope, noted for transparency)
Two other copies of the same hardcoded allowlist live in
comfy_cli/command/jobs.py— thecomfy jobs listoutput-count display (~L325) and thecomfy jobs watchWebSocketexecutedhandler (~L1195). Both are on the local-server command surface, not the cloudcomfy downloadpath this ticket targets, so they are deliberately left untouched to keep the diff scoped. They would exhibit the same 3D-blindness for local jobs; a follow-up can converge them on the same shape-based detection if desired.Closes #508 (spike BE-4278).